feat(motor): add readback tolerance check to EpicsMotor.move() - #107

What is the reasoning behind adding this class instead of relying on PVPositionerIsClose or PVPositionerComparator like we usually do? In other terms, what does it provide for us that the other method does not already provide with no extra maintenance burden?

Also, does this behavior occur in a specific IOC device? Since this is clearly a bug, it'd be nice if we could have it fixed in the lower level instead of working around it here.

What is the reasoning behind adding this class instead of relying on PVPositionerIsClose or PVPositionerComparator like we usually do? In other terms, what does it provide for us that the other method does not already provide with no extra maintenance burden?

Also, does this behavior occur in a specific IOC device? Since this is clearly a bug, it'd be nice if we could have it fixed in the lower level instead of working around it here.

I couldn't find a straightforward way to use PVPositionerIsClose in this case. I ran a few tests, but using PVPositionerIsClose directly meant losing the EpicsMotor structure.

With this approach, I think we can better preserve the EpicsMotor behavior while still adding the required check. It also allows us to use it directly in our common motor devices.

But we can talk to find a better way to implement this. What do you think?

Regarding the IOC, I encountered this issue with the CNB motors using DeltaTau (PB01, PB02, etc.) and with the new controller for the 4CM (CK5M01). I can confirm with Ana and the IOC team whether this bug also occurs in other contexts.

However, I still think it would be useful to keep both checks, using .RBV and .DMOV, to make the position verification more robust.

What is the reasoning behind adding this class instead of relying on PVPositionerIsClose or PVPositionerComparator like we usually do? In other terms, what does it provide for us that the other method does not already provide with no extra maintenance burden?
Also, does this behavior occur in a specific IOC device? Since this is clearly a bug, it'd be nice if we could have it fixed in the lower level instead of working around it here.

I couldn't find a straightforward way to use PVPositionerIsClose in this case. I ran a few tests, but using PVPositionerIsClose directly meant losing the EpicsMotor structure.

With this approach, I think we can better preserve the EpicsMotor behavior while still adding the required check. It also allows us to use it directly in our common motor devices.

But we can talk to find a better way to implement this.

Regarding the IOC, I encountered this issue with the CNB motors using DeltaTau (PB01, PB02, etc.) and with the new controller for the 4CM (CK5M01). I can confirm with Ana and the IOC team whether this bug also occurs in other contexts.

However, I still think it would be useful to keep both checks, using .RBV and .DMOV, to make the position verification more robust.

I think the main issue with using the RBV value for checking movement state is that you become less robust on other issues, like if the motor gets stuck for some reason. In those cases, the DMOV flag correctly indicates that the movement is done, and an alarm or error PV indicates some failure. With the RBV check, we can no longer trust that, and the worst case becomes timing out, which is also a very unreliable thing to configure since it changes from motor to motor.

This can be seen in this piece of code from EpicsMotor itself, where the self._moving flag (relative to DMOV) is checked, before checking the alarm state and then deciding on what the user will get. With your AndStatus implementation, that will only be propagated after the RBV status has timed out, which for the inattentive user may be never.

Aside from that consideration, another important thing is that if the DMOV signal is not reliable, then in some cases the RBV check will also be unreliable. Consider a PID motor that goes through a position settling moment before finishing the movement. In this case, the DMOV should tell us when the movement has actually stopped, but if you can't do that, the next best thing is to add an arbitrary settle time to the position monitor for that ending movement to be captured correctly, but that can either add some amount of dead time, or it can be insufficient, and errors can arise in hard-to-foresee scenarios, like a race condition between the motor controller and the orchestration layer.

What is the reasoning behind adding this class instead of relying on PVPositionerIsClose or PVPositionerComparator like we usually do? In other terms, what does it provide for us that the other method does not already provide with no extra maintenance burden?
Also, does this behavior occur in a specific IOC device? Since this is clearly a bug, it'd be nice if we could have it fixed in the lower level instead of working around it here.

I couldn't find a straightforward way to use PVPositionerIsClose in this case. I ran a few tests, but using PVPositionerIsClose directly meant losing the EpicsMotor structure.
With this approach, I think we can better preserve the EpicsMotor behavior while still adding the required check. It also allows us to use it directly in our common motor devices.
But we can talk to find a better way to implement this.
Regarding the IOC, I encountered this issue with the CNB motors using DeltaTau (PB01, PB02, etc.) and with the new controller for the 4CM (CK5M01). I can confirm with Ana and the IOC team whether this bug also occurs in other contexts.
However, I still think it would be useful to keep both checks, using .RBV and .DMOV, to make the position verification more robust.

I think the main issue with using the RBV value for checking movement state is that you become less robust on other issues, like if the motor gets stuck for some reason. In those cases, the DMOV flag correctly indicates that the movement is done, and an alarm or error PV indicates some failure. With the RBV check, we can no longer trust that, and the worst case becomes timing out, which is also a very unreliable thing to configure since it changes from motor to motor.

This can be seen in this piece of code from EpicsMotor itself, where the self._moving flag (relative to DMOV) is checked, before checking the alarm state and then deciding on what the user will get. With your AndStatus implementation, that will only be propagated after the RBV status has timed out, which for the inattentive user may be never.

Aside from that consideration, another important thing is that if the DMOV signal is not reliable, then in some cases the RBV check will also be unreliable. Consider a PID motor that goes through a position settling moment before finishing the movement. In this case, the DMOV should tell us when the movement has actually stopped, but if you can't do that, the next best thing is to add an arbitrary settle time to the position monitor for that ending movement to be captured correctly, but that can either add some amount of dead time, or it can be insufficient, and errors can arise in hard-to-foresee scenarios, like a race condition between the motor controller and the orchestration layer.

That makes sense Sofia. In cases where the motor gets stuck for some reason, we would have to rely on the configured timeout, which can vary from motor to motor. What do you think about implementing a check for self._moving in this new class, similar to what EpicsMotor does, and using that information to check the alarm/error state and report the movement failure to the user, before considering the .RBV? In any case, I’ll bring this discussion to the Orchestration meeting so we can try to come up with a solution for this issue.